Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[move-stdlib] Use vector::move_range inside vector, and evaluate performance / calibrate gas #14862

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

[move-stdlib] Use vector::move_range inside vector, and evaluate performance / calibrate gas #14862

wants to merge 17 commits into from

Conversation

igor-aptos
Copy link
Contributor

@igor-aptos igor-aptos commented Oct 3, 2024 •
edited
Loading

Description

Use vector::move_range inside of vector, to optimize insert, remove, append, trim.
Extend aptos-move/e2e-benchmark/src/main.rs to track gas and gas/s, to allow for quick calibration.
Adding workloads to txn-emitter to be able to use it throughput.

Additionally add a missing replace method, which replaces value at particular index.

Running on extended set of params:
https://gist.github.com/igor-aptos/e8d4e21edcbc75dddcb9382d4e077665

Summary of the performance tests:

  • performance doesn't depend on the size of the values (unless they are primitive), as vector modifies only pointers to them.
  • operation depends very little on how many elements we need to move - moving 1000 elements (i.e. to insert into a vector 1000 elements from the end) is only (!!) 2x slower than moving 1 element, end-to-end.

For gas calibration, on the variety of workloads, current implementation has decent variance. After tuning params to match the averages, variance seems much smaller.

How Has This Been Tested?

Key Areas to Review

Type of Change

  • Performance improvement

Which Components or Systems Does This Change Impact?

  • Aptos Framework

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@trunk-io Trunk.io
Copy link

trunk-io bot commented Oct 3, 2024 •
edited
Loading

⏱️ 8h 45m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 4h 8m ⬜ 🟥 🟥 🟥 🟥 (+2 more)
execution-performance / test-target-determinator 25m 🟩 🟩 🟩 🟩 🟩 (+2 more)
test-target-determinator 23m 🟩 🟩 🟩 🟩 🟩 (+2 more)
rust-move-unit-coverage 16m 🟩 
rust-move-unit-coverage 16m 🟩 
rust-move-unit-coverage 15m 🟩 
check-dynamic-deps 15m 🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-cargo-deny 14m 🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-move-unit-coverage 11m 🟩 
rust-move-unit-coverage 10m 🟩 
rust-move-unit-coverage 10m 🟩 
rust-move-tests 10m 🟥 
rust-move-tests 10m 🟥 
rust-move-tests 10m 🟩 
rust-move-unit-coverage 10m 🟩 

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
execution-performance / single-node-performance 36m 16m +121%

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@igor-aptos igor-aptos mentioned this pull request Oct 3, 2024
Merged
8 tasks
@codecov Codecov
Copy link

codecov bot commented Oct 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (igor/native_vector_move_range@b36187b). Learn more about missing BASE report.

Additional details and impacted files 
@@                       Coverage Diff                        @@
##             igor/native_vector_move_range   #14862   +/-   ##
================================================================
  Coverage                                 ?    60.1%           
================================================================
  Files                                    ?      858           
  Lines                                    ?   211455           
  Branches                                 ?        0           
================================================================
  Hits                                     ?   127237           
  Misses                                   ?    84218           
  Partials                                 ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@igor-aptos igor-aptos force-pushed the igor/native_vector_move_range branch from 75ebf46 to d724063 Compare October 4, 2024 16:59
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from 0df908c to 0fab436 Compare October 4, 2024 17:00
@igor-aptos igor-aptos added CICD:run-execution-performance-test Run execution performance test CICD:run-execution-performance-full-test Run execution performance test (full version) labels Oct 4, 2024
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from 0fab436 to b1a2e70 Compare October 4, 2024 22:36
@igor-aptos igor-aptos mentioned this pull request Oct 8, 2024
Closed
8 tasks
@igor-aptos igor-aptos force-pushed the igor/native_vector_move_range branch from d724063 to e4540db Compare October 8, 2024 19:00
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from b1a2e70 to 6b493cd Compare October 8, 2024 19:00
@igor-aptos igor-aptos force-pushed the igor/native_vector_move_range branch from e4540db to ae8e817 Compare October 9, 2024 20:30
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from 6b493cd to 6046016 Compare October 9, 2024 20:30
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from 6046016 to c5e50b3 Compare October 10, 2024 00:08
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from c5e50b3 to 36f8618 Compare October 10, 2024 18:10
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from 36f8618 to 9851f51 Compare October 15, 2024 19:48
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from 9fcf365 to 53920f9 Compare November 8, 2024 23:25
@igor-aptos igor-aptos changed the base branch from igor/native_vector_move_range to igor/constant_serialized_size November 8, 2024 23:26
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from 53920f9 to 58c55c0 Compare November 9, 2024 00:17
@igor-aptos igor-aptos force-pushed the igor/use_native_vector_move_range branch from 58c55c0 to e1f971f Compare November 12, 2024 18:04
msmouse
msmouse approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@msmouse msmouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(needs rebase, looked at what the summary says)

types/src/on_chain_config/aptos_features.rs
@@ -326,7 +326,7 @@ impl Features {
pub fn is_loader_v2_enabled(&self) -> bool {
self.is_enabled(FeatureFlag::ENABLE_LOADER_V2)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove

@igor-aptos igor-aptos force-pushed the igor/constant_serialized_size branch 2 times, most recently from 8321a01 to 974ad93 Compare November 13, 2024 19:32
igor-aptos added a commit that referenced this pull request Nov 13, 2024
@igor-aptos
[move-stdlib] Add vector::move_range native function (#14863)
61e6563 
## Description

Memcopy (i.e. `ptr::copy_nonoverlapping` inside of `Vec`) is extremely efficient, and using Vec operations that use it directly is significantly faster (orders of magnitude on bigger vectors) than issuing operations in move.

Operations on `vector` that can be speed-up: `insert`, `remove`, `append`, `split_off`.

To keep amount of native functions short, instead of having native for each of those, providing one more general native function: `vector::move_range`, which is enough to support all 4 of the above, in addition to other uses. 

Internally, we shortcircuit a few special cases, for faster speed.

## How Has This Been Tested?
Full performance evaluation is in the follow-up PR: #14862

## Type of Change
- [x] Performance improvement

## Which Components or Systems Does This Change Impact?
- [x] Move/Aptos Virtual Machine
Base automatically changed from igor/constant_serialized_size to main November 14, 2024 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msmouse msmouse msmouse approved these changes

@lightmark lightmark Awaiting requested review from lightmark lightmark is a code owner

@vgao1996 vgao1996 Awaiting requested review from vgao1996

@wrwg wrwg Awaiting requested review from wrwg wrwg is a code owner

@gelash gelash Awaiting requested review from gelash

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@movekevin movekevin Awaiting requested review from movekevin movekevin is a code owner

@zekun000 zekun000 Awaiting requested review from zekun000

@georgemitenkov georgemitenkov Awaiting requested review from georgemitenkov

@grao1991 grao1991 Awaiting requested review from grao1991 grao1991 is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
CICD:run-execution-performance-full-test Run execution performance test (full version) CICD:run-execution-performance-test Run execution performance test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@igor-aptos @msmouse